src/Altinn.Correspondence.Core/Repositories/IAltinnAuthorizationService.cs (1)

8-8: Ensure secure handling of SSN.

The method now accepts SSN (Social Security Number) which is sensitive PII data. Ensure all implementations:

1. Use secure transport (HTTPS)
2. Implement proper logging practices (no SSN in logs)
3. Follow data protection regulations

src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationDevService.cs (1)

20-20: Document the significance of auth level 3

Consider extracting the magic number 3 to a named constant and documenting its significance in the context of authentication levels.

+ private const int DefaultDevAuthLevel = 3; // Represents highest security level for development
- return Task.FromResult((int?)3);
+ return Task.FromResult((int?)DefaultDevAuthLevel);

src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs (3)

115-115: Update method documentation to reflect signature change

The AuthorizeRequest method now accepts a XacmlJsonRequestRoot parameter. Ensure that any documentation or XML comments for this method are updated to reflect this change for clarity.

---

149-157: Refactor to eliminate code duplication in decision request methods

The methods CreateDecisionRequest and CreateDecisionRequestForLegacy share similar logic regarding personIdClaim and issuer checks. Consider refactoring shared logic into a private helper method to reduce duplication and improve maintainability.

Apply this diff to refactor the shared logic:

+ private bool IsAltinnTokenIssuer(Claim? personIdClaim)
+ {
+     return personIdClaim is null || personIdClaim.Issuer == $"{_altinnOptions.PlatformGatewayUrl.TrimEnd('/')}/authentication/api/v1/openid/";
+ }

- if (personIdClaim is null || personIdClaim.Issuer == $"{_altinnOptions.PlatformGatewayUrl.TrimEnd('/')}/authentication/api/v1/openid/")
+ if (IsAltinnTokenIssuer(personIdClaim))
{
    // Existing logic
}

---

152-156: Consider more informative exception handling

In CreateDecisionRequestForLegacy, throwing a generic SecurityTokenInvalidIssuerException may not provide sufficient context. Consider throwing a more specific exception or including a descriptive message to aid in debugging.

Apply this diff to enhance exception handling:

- throw new SecurityTokenInvalidIssuerException();
+ throw new SecurityTokenInvalidIssuerException($"Unsupported issuer: {personIdClaim?.Issuer}");

Configuration used: CodeRabbit UI
Review profile: CHILL

Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#413
File: src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryResponse.cs:17-21
Timestamp: 2024-10-29T13:10:17.164Z
Learning: The `PartyId` and `AuthenticationLevel` properties in the `LegacyUser` class (in `src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryResponse.cs`) are still under development, and the correct validation has not been determined yet.

#!/bin/bash
# Description: Check for implementations of IAltinnAuthorizationService
# and verify they implement the new method signature correctly

# Find all implementations
ast-grep --pattern 'class $_ : $$$IAltinnAuthorizationService$$$'

# Find all calls to CheckUserAccessAndGetMinimumAuthLevel
rg -A 3 'CheckUserAccessAndGetMinimumAuthLevel'

#!/bin/bash
# Description: Check if this development service is referenced in production configurations
# Expected: No matches in production config files

# Search for references to AltinnAuthorizationDevService in config files
rg -l "AltinnAuthorizationDevService" | rg -i "prod|release|deployment"

#!/bin/bash
# Let's try a more comprehensive verification approach:

# 1. Find all references to this service class
echo "=== Direct references to AltinnAuthorizationDevService ==="
rg "AltinnAuthorizationDevService"

# 2. Check where IAltinnAuthorizationService is registered in DI
echo -e "\n=== DI registrations for IAltinnAuthorizationService ==="
rg "IAltinnAuthorizationService" -A 5 -B 5

# 3. Look for environment-specific configuration files
echo -e "\n=== Environment configurations ==="
fd -e json -e xml "appsettings|config" --exec grep -l "Authorization"

#!/bin/bash
# Description: Analyze the usage of legacy authorization methods and potential security implications

# Test 1: Find all usages of the legacy decision request method
echo "=== Legacy Decision Request Usage ==="
rg -A 5 "CreateAltinnDecisionRequestForLegacy"

# Test 2: Check for potential hardcoded SSNs or sensitive data
echo -e "\n=== Potential Sensitive Data ==="
rg -i "ssn|socialsecurity|personnumber"

# Test 3: Analyze authorization service implementations
echo -e "\n=== Authorization Service Usage ==="
ast-grep --pattern 'class $_ : IAltinnAuthorizationService {
  $$$
  CheckUserAccessAndGetMinimumAuthLevel($$$) {
    $$$
  }
  $$$
}'

#!/bin/bash
# Description: Verify that all calls to CheckUserAccessAndGetMinimumAuthLevel include the new 'ssn' parameter.

# Test: Search for all invocations of the method. Expect: All calls should include 'ssn' as the first argument.
rg 'CheckUserAccessAndGetMinimumAuthLevel' --type cs -C 2

- if (personIdClaim is null || personIdClaim.Issuer == /* condition */)
+ if (personIdClaim == null || personIdClaim.Issuer == /* condition */)
{
    // Existing logic
}

src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs (2)

60-68: Consider making the minimum authentication level configurable.

The method returns a hard-coded authentication level of 3 when validation succeeds. This might need to be configurable based on different security requirements or environments.

Consider:

1. Moving this to configuration
2. Making it dependent on the resource type or access level requested
3. Adding documentation explaining why level 3 was chosen

---

Line range hint 115-133: LGTM! Consider enhancing error details in response.

The error handling is comprehensive, but we could improve the logging of failure responses.

Consider adding more detailed error logging:

 if (!response.IsSuccessStatusCode)
 {
+    var errorContent = await response.Content.ReadAsStringAsync(cancellationToken);
+    _logger.LogError("Authorization request failed. Status: {StatusCode}, Error: {Error}",
+        response.StatusCode, errorContent);
     return null;
 }

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Check for proper token validation across the codebase
echo "Checking token validation patterns..."
rg -A 5 "ValidateToken|ValidateAsync|SecurityToken" 

echo "Checking authentication middleware configuration..."
rg -A 10 "UseAuthentication|AddAuthentication"

echo "Checking for potential authentication bypasses..."
ast-grep --pattern 'if.*IsDevelopment.*return true'

#!/bin/bash
# Search for all calls to CreateDecisionRequest to ensure consistent usage
ast-grep --pattern 'CreateDecisionRequest($$$)'

#!/bin/bash
# Search for CreateDecisionRequest method definition and its usages
ast-grep --pattern 'CreateDecisionRequest($$$)'
# Also search with ripgrep for better context
rg -A 5 'CreateDecisionRequest'

src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnTokenXacmlMapper..cs (1)

71-81: Consider refactoring to enhance code maintainability

The CreateResourceCategory method now includes additional parameters legacy and orgNr, introducing conditional logic based on the legacy flag. To improve readability and maintainability, consider separating the legacy and non-legacy logic into distinct methods or refactoring the method to reduce complexity.

Configuration used: CodeRabbit UI
Review profile: CHILL

private static XacmlJsonCategory CreateSubjectCategoryForLegacy(ClaimsPrincipal user, string ssn)
{
+   if (!IsValidNorwegianSSN(ssn))
+   {
+       throw new ArgumentException("Invalid SSN format", nameof(ssn));
+   }

    XacmlJsonCategory xacmlJsonCategory = new XacmlJsonCategory();
    List<XacmlJsonAttribute> list = new List<XacmlJsonAttribute>();
    var claim = user.Claims.FirstOrDefault(claim => IsScopeClaim(claim.Type));
    if (claim is not null)
    {
        list.Add(CreateXacmlJsonAttribute("urn:altinn:person:identifier-no", ssn, "string", claim.Issuer));
        list.Add(CreateXacmlJsonAttribute("urn:scope", claim.Value, "string", claim.Issuer));
    }
    xacmlJsonCategory.Attribute = list;
    return xacmlJsonCategory;
}

+private static bool IsValidNorwegianSSN(string ssn)
+{
+    // Norwegian SSN validation logic
+    return Regex.IsMatch(ssn, @"^\d{11}$");
+}